Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ferm::rule parameters outerface, to_source and to_destination. #111

Closed
wants to merge 4 commits into from
Closed

Add ferm::rule parameters outerface, to_source and to_destination. #111

wants to merge 4 commits into from

Conversation

robinelfrink
Copy link

@robinelfrink robinelfrink commented Jun 22, 2020 •
edited
Loading

Pull Request (PR) description

Add options:

  • outerface (iptables --out-interface)
  • to_source (iptables --to-source)
  • to_destination (iptables --to-destination)

This Pull Request (PR) fixes the following issues

Fixes #74

@bastelfreak bastelfreak added enhancement New feature or request needs-tests labels Jun 22, 2020
@bastelfreak bastelfreak requested a review from foxxx0 June 22, 2020 08:19
@bastelfreak
Copy link
Member

@robinelfrink thanks for the PR! can you please add unit and maybe an acceptance test for this?

@robinelfrink
Copy link
Author

@robinelfrink thanks for the PR! can you please add unit and maybe an acceptance test for this?

I'll do my best :)

@robinelfrink robinelfrink force-pushed the rule-add-outerface-and-to_source branch 2 times, most recently from c671162 to 676d5cf Compare June 22, 2020 09:05
@robinelfrink
Copy link
Author

@bastelfreak Tests added.

foxxx0
foxxx0 requested changes Jun 22, 2020
View reviewed changes
manifests/rule.pp Outdated Show resolved Hide resolved
manifests/rule.pp Outdated Show resolved Hide resolved
spec/defines/rule_spec.rb Outdated Show resolved Hide resolved
manifests/rule.pp Show resolved Hide resolved
@foxxx0 foxxx0 self-assigned this Jun 22, 2020
foxxx0
foxxx0 reviewed Jun 22, 2020
View reviewed changes
manifests/rule.pp Outdated
@@ -162,7 +162,7 @@
$filename = "${ferm::configdirectory}/chains/${table}-${chain}.conf"
}

$rule = squeeze("${comment_real} ${proto_real} ${proto_options_real} ${dport_real} ${sport_real} ${daddr_real} ${saddr_real}${outerface_real} ${action_real}${to_source_real};", ' ')
$rule = join([strip(squeeze("${comment_real} ${proto_real} ${proto_options_real} ${dport_real} ${sport_real} ${daddr_real} ${saddr_real} ${outerface_real} ${action_real} ${to_source_real}", ' ')), ';'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we now suddenly need join() and strip()?
From what I can tell you're only adding strings and everyting is inside a single string interpolation anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because to ... needs to be after SNAT, and <space>${to_source_real} results in a single space between ${action_real} and ; in all rules that have an empty ${to_source_real}.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That extra space makes all (other) acceptance tests fail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could make it more clear using a regsubst() maybe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide compiled/expanded results for each?
I.e. output for the version that doesn't work and output for the one that does work. I'm having a little trouble seeing why any of that regsubst() or similar is needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of why tests fail when a rule ends with <space><semi colon>:

  84) ferm::rule on centos-7-x86_64  definining rules in custom chains is expected to contain Concat::Fragment[SSH-allow-ssh-localhost] with content  supplied string
      Failure/Error: it { is_expected.to contain_concat__fragment('SSH-allow-ssh-localhost').with_content("mod comment comment 'allow-ssh-localhost' proto tcp dport 22 saddr @ipfilter((127.0.0.1)) ACCEPT;\n") }
      
        expected that the catalogue would contain Concat::Fragment[SSH-allow-ssh-localhost] with content set to supplied string
        Diff:
        @@ -1,2 +1,2 @@
        -mod comment comment 'allow-ssh-localhost' proto tcp dport 22 saddr @ipfilter((127.0.0.1)) ACCEPT;
        +mod comment comment 'allow-ssh-localhost' proto tcp dport 22 saddr @ipfilter((127.0.0.1)) ACCEPT ;

@robinelfrink
Copy link
Author

Oh, by the way, should I do some squashing first?

@foxxx0
Copy link
Member

foxxx0 commented Jun 22, 2020

That would be great, yes.

@robinelfrink robinelfrink force-pushed the rule-add-outerface-and-to_source branch from 8ff18f0 to d374fc6 Compare June 22, 2020 17:34
@robinelfrink robinelfrink force-pushed the rule-add-outerface-and-to_source branch from d374fc6 to 4f2cc4d Compare June 23, 2020 06:22
@robinelfrink robinelfrink force-pushed the rule-add-outerface-and-to_source branch from 23933ad to ecc8d22 Compare June 24, 2020 13:03
@robinelfrink robinelfrink changed the title Add ferm::rule parameters outerface and to_source. Add ferm::rule parameters outerface, to_source and to_destination. Jun 24, 2020
@robinelfrink
Copy link
Author

I've also added to_destination for use with DNAT.

Travis keeps complaining about dport needing to be of a specific type, but I do not see how that's only with my changes since some existing tests have exactly the same parameter value. I can really use some help with this.

@vox-pupuli-tasks
Copy link

Dear @robinelfrink, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

2 similar comments
@vox-pupuli-tasks
Copy link

Dear @robinelfrink, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @robinelfrink, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @robinelfrink, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foxxx0 foxxx0 foxxx0 requested changes

@bastelfreak bastelfreak bastelfreak approved these changes

Assignees

@foxxx0 foxxx0

Labels
enhancement New feature or request merge-conflicts tests-fail
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

outerface?
3 participants
@robinelfrink @bastelfreak @foxxx0